💾 Iobio Charts @19 #39
Conversation
|
|
||
| if (element && styles) { | ||
| setElementStyles(element, styles); | ||
| } |
There was a problem hiding this comment.
There is a follow up ticket to improve the styles utility: #27
There was a problem hiding this comment.
Awesome!
PS: Took the liberty to modify your comment to replace the ZenHub link with a GitHub one, so external collaborators and integrators can follow progress. (they don't have acccess otherwise)
| @@ -26,12 +26,12 @@ function IobioHistogram({ | |||
| brokerKey, | |||
| styles, | |||
| ignoreOutliers = false, | |||
| title, | |||
| label, | |||
There was a problem hiding this comment.
it may seem like nitpicking, but it's highly recommended to keep properties alphabetised whenever possible.
not a change required for this PR, but something to keep in mind for future changes.
There was a problem hiding this comment.
I generally try to do that, guess I overlooked this one
There was a problem hiding this comment.
here and in a few other places. don't worry too much about it for now.
it's more DevX than functionality, so it's not an urgent thing to address.
| }) { | ||
| useEffect(() => { | ||
| const selector = `iobio-percent-box[percent-key=${percentKey}][total-key=${totalKey}]`; | ||
| const element = document.querySelector(selector); |
There was a problem hiding this comment.
react refs might be useful here:
- no need to worry about multiple elements
document.querySelectorwill return one element but what if the selector accidentally target multiple elements)
- avoids searching the DOM for an element
- inbuilt React library code for targeting DOM elements
- react will handle updating val to null if element is not available
- doesn't depend on
documentapi being available (eg. for SSR)
Updates Iobio Charts to v0.19